Value methods to assert value kind #50
Conversation
|
Nice! This looks to be a precursor to being able to extract the values (beyond the existing string method)? |
|
That's right; once I have this PR done, I'll add the |
|
Hey @tmc Would you mind giving this a sanity check? |
.github/workflows/test.yml
Outdated
| @@ -1,6 +1,11 @@ | |||
| name: CI | |||
|
|
|||
| on: [push, pull_request, workflow_dispatch] | |||
| on: | |||
There was a problem hiding this comment.
did you mean to change this to only run on master? it seems like this should run on any push.
nit: whitespace at EOL
There was a problem hiding this comment.
This is a quirk of GitHub actions. I previously had just push but that does not work for pushes to forks, so I added pull_request but now if I (and only people that can push directly to this repo) submit a pull request the tests get run twice: Once for push and again for pull_request.
There should be no other branches other than master that should need to run the test unless you are creating a pull request. If you really need to then that's what workflow_dispatch will allow, but is a manual process.
v8go.cc
Outdated
| void ValueDispose(ValuePtr ptr) { | ||
| delete static_cast<m_value*>(ptr); | ||
| delete static_cast<m_value*>(ptr); |
There was a problem hiding this comment.
would love to get clangtidy/otherwise in here to standardize the non-go code formatting as well.
There was a problem hiding this comment.
Agree; I did format the whole file, but I thought that was too much of a change to add to this PR, so I reverted that. (missed this line though).
I'll look to add a clang-format in a future PR, but if you're ok having a larger PR, then I'll just run my default format for now? or leave as-is? @tmc
There was a problem hiding this comment.
#51 I guess it depends which PR you approve first 😉
Expanding the API for the
Valuestruct to include checks for the value returned by a run script.This will pave the way forward for more API's to interact between Go and Javascript.